Skip to content

Conversation

@johuder33
Copy link

@johuder33 johuder33 commented Oct 28, 2022

This package works perfectly well, but when you use it in a service worker context, it failed because window.* access is not support by service worker.

So in order to support this package into service worker, we should make a little change, replace any window.* usage by self.*.

So self will work for non-window contexts where this code can be executed, so with this change this package can be used into a service worker without any problem 😄 .

if window.addEventListener worry you, it is well-covered, since before adding some listener to specific events, the code is checking if the context it's actually a window one by typeof window execution.

@travist
Copy link
Owner

travist commented Nov 2, 2022

A change like this really needs some automated tests around it. More specifically I would like to know if the "addEventListener" methods and things referencing the "self" object are indeed referencing the "window" object in a browser environment.

@johuder33
Copy link
Author

johuder33 commented Nov 3, 2022

Hello @travist , thank you for taking some time and read this PR, sure I can add some test in order to cover this change, please let me know which type of test you think is better to implement, also, the self variable will point to the main object based on the environment where the code is executed, for example, if you execute the code in a normal web environment it will point to window, but if it is executed into a Service Worker it will point to the ServiceWorkerGlobalScope, but please let me know how can I help in this.

@MaxNoetzold
Copy link

I would love to have this feature as well.
In principle, I see the point of automated tests, but it doesn't really seem necessary in this case. As mdn states window and self are equal in the browser context:

const w1 = window;
const w2 = self;
const w3 = window.window;
const w4 = window.self;
// w1, w2, w3, w4 all strictly equal, but only w2 will function in workers

However, I would love to help get it approved if there is anything for me to do.

@travist travist merged commit a9865b4 into travist:master Aug 10, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants